Why use a client-side timeout instead of plumbing contexts?

Removed the client-side timeout

@bennerv should we still do a context.WithTimeout to be explicit about how long we want to wait here? otherwise we just get we the request context defines.

Haven't added the context.WithTimeout yet, can wait for the decision. Or, handle it in the middleware at some point if we want to have a default request timeout?

My vote:

1. We 100% should add middleware with some kind of timeout (5 minutes?) as a stop-gap
2. As needed for optimization, individual handlers can/should restrict that timeout even further to exit earlier so SREs aren't kept waiting on reasonably-known failure states for a particular task.

Initially in the PR this was being used by the serialconsolelogs implementation, but since it no longer uses it, this can be moved back to breakglass. But decided to keep it here for any similar future use.
Please comment if anyone feel against moving the function here in this PR.

nit: as an SRE working a 2am incident - if I saw this error message raw I might be confused and need to review the code to understand the error. Adding a bit of clarity to what this error actually means (and how to resolve it?) for on-call could be helpful if that's even possible. I think this is especially true considering we never expect diagnostics to be off - the oddest cases likely deserve the clearest messaging.

nit: should we write a warning or something if data is truncated at 100MB? Note - this truncates to the beginning rather than the end of the content. Should we instead return the end of the log?

Good suggestion, I will address in the next PR where I can just handle the streaming of the logs.

bug: once you write a header - it can NEVER be overwritten. Your errors at L183 are silently ignored and go to logs - but SREs are getting 200/OK (and potentially getting partial data) by putting this here.

I think the best way for us to handle this is to buffer the full content and only return once we have the data in-hand. if we're capping at 100MB I think this is fine?

Yes, that's true and that's the idea when streaming the logs.

I guess I meant - streaming can fail, and there's not a great way to handle those failures and notify on-call of the failures. Considering we're only dealing with 100MB, it might be best to eliminate streaming all-together, and only return once we've got the full data to return safely with 200. That has implications on memory for admin-api for sure - but it seems reasonable - and I don't think we expect to call this endpoint often (e.g. I don't see us invoking this on every ICM; it's just a critical gap we need to fill in rare situations).

ultimately it's not a huge deal either way - the scenarios are slim - but considering that (silent) failures here could lead to incorrect assumptions in triage of incidents, it seems important to be more correct than resource-aware?

could we save a cluster create and NP create cycle by adding the "fetch VM -> disable -> 409" to the previous suite? I guess that conflates positive/negative ... but this feels heavy for a single HTTP test. Could it be an integration test?

I believe I thought of doing it in a single suite, but separated it as the disabling one requires an external action on the VM to disable the boot diagnostics which might not work in the envs where there's deny assignment in place. But the previous suite would work in any environment.